Add GitHub API importer - #204

What does this means exactly?What about using a cascade of get() instead to understand exactly why there are these key errors? Can you show me the problematic JSON?

Can you show me the problematic JSON?

This is raising that exception https://api.nuget.org/v3/registration5-semver1/yamldotnet.signed/index.json
All others follow this schema instead https://api.nuget.org/v3/registration5-semver1/sustainsys.saml2/index.json

What about using a cascade of get() instead to understand exactly why there are these key errors?

That can be done, but I thought catching an exception would be more 'pythonic' (EAFP).

That can be done, but I thought catching an exception would be more 'pythonic' (EAFP).

Well, the key is readability and clarity first: here when I read the code in json_resp['items'][0]['items'] I wonder immediately. What if there is no items or this is not a mapping? What is [0]? Is this a list or a mapping with a key 0? if this is a list, what about the other items in that list? and what is this second items about?

Here we could have something instead like that:

for item in json_resp.get('items', []):
    for entry in item.get('items', []):
        ...

When I read it I get these are mapping that return lists and I have no problem to understand the underlying data structure that I do not know about. Otherwise, I feel I need to do quite a bit of effort to read the code and it assumes that I know about the upstream JSON data structure before hand. So EAFP is fine, but it matters more to think about the readability of the code IMHO and how it will resist to future changes in the API and how that will make it easy to debug or not.

This is raising that exception https://api.nuget.org/v3/registration5-semver1/yamldotnet.signed/index.json
All others follow this schema instead https://api.nuget.org/v3/registration5-semver1/sustainsys.saml2/index.json

This is the kind of comment that is needed in the code :)

I agree with everything you said.

I am wondering what's the best answer in this context to the question you asked.

how it will resist to future changes in the API and how that will make it easy to debug or not.

Because in this case, during an occurence of API change the outcome of both approaches EAFP(try-except) and LBYL(get cascade) is same. The code won't crash in an event of API changes in both the cases, resulting in not collecting the data, and that will go unnoticed, unless someone decides to look into the db.

1. you can still keep inside your direct items access as all_versions.add(entry['catalogEntry']['version']) so if that part changes if will fail.
2. we should have tests too
3. we could think about a very limited subset of "canary"-like tests that are doing live queries on the web and fail if the API changes by returning different data.

A pass would be clearer... BUT the reason for swallowing the KeyError is what I would like to understand

I have no idea how to handle https://api.nuget.org/v3/registration5-semver1/yamldotnet.signed/index.json which is causing the exception, hence the 'swallowing' of the KeyError

always prefer a more robust partition() or rpartition():

>>> vendor, name = pkg_name.split('/')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'pkg_name' is not defined
>>> vendor, _, name = 'foo'.partition('/')
>>> vendor, name
('foo', '')
>>> vendor, _, name = 'foo/bar'.partition('/')
>>> vendor, name
('foo', 'bar')

always prefer a more robust partition() or rpartition():

I didn't knew about this function. Thanks.
Robust in the sense it doesn't raise ValueError: not enough values to unpack .

But in this case it makes more sense to me to use split because that would raise an exception and crash out VulnerableCode, instead of ending up writing garbage in the DB.

nit: Envirnomental -> Environment

Fixed

May be you could reuse that function (with a comment you did) instead?
https://github.com/nexB/scancode-toolkit/blob/develop/src/packagedcode/maven.py#L225
This way we can eventually evolve a common maven library out of ScanCode
https://repo.maven.apache.org/maven2 is not the most common base URL too

that's minor though and could wait

In general it is often better to yield rather than to accumulate in a list, FWIW

Same as above... yielding rather than returning a list is always a good option